Hi @jcferretti. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/retest

@jcferretti: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

/ok-to-test

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jcferretti
Once this PR has been reviewed and has the lgtm label, please assign jmhbnz for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

There have been no comments in this PR for two months, and on a separate PR a vague comment was made about the appropriateness of the tests and that being the reason for it being held.

It would be good to hear, directly here, concrete points about what is believed to be lacking in the existing tests, and what else specifically would be important to have to move this PR forward.

Thanks.

Discussed during sig-etcd triage meeting, @ahrtr could you please take a look at this pr and provide some guidance on next steps, thanks!

Thanks for the PR! It's a bit hard to follow, and it's also missing comprehensive e2e and integration tests. But see more important comments below.

I think we have two approaches to fix it:

• Do not change the golang source code. Instead, we just need to update the document to clarify that Count reflects the number of keys within the range, without considering the filters (Min or Max, Create or Modify, Revisions).
  

  etcd/api/etcdserverpb/rpc.proto

  Lines 510 to 511 in 68e7122

  	// count is set to the number of keys within the range when requested.
  	int64 count = 4;

• If we really want the Count also reflects the filters, then we should get the filters (Min or Max, Create or Modify, Revisions) included in RangeOptions, see

  etcd/server/storage/mvcc/kv.go

  Lines 26 to 30 in 68e7122

  	type RangeOptions struct {
  	Limit int64
  	Rev int64
  	Count bool
  	}

  So the returned data already is already the filtered data.
  

  etcd/server/etcdserver/txn/txn.go

  Line 176 in 68e7122

  rr, err := txnRead.Range(ctx, r.Key, mkGteRange(r.RangeEnd), ro)

  Obviously it has a little big impact on the most important part of etcdserver. We may even need to update the API Revisions. So it definitely needs super careful review.

  etcd/server/storage/mvcc/index.go

  Line 27 in 68e7122

  Revisions(key, end []byte, atRev int64, limit int) ([]Revision, int)

@serathius What do you think?

BTW, FYI the filters were introduced in #6411 and #6424

From my perspective I would be ok (and I think in favor) of just updating the documentation to be explicit on what count is returned in this case and then not changing the code (meaning closing this PR without implementing it).

All the options and what any particular combination means can get pretty complicated from a semantics perspective. I think the API is already breaking the principle of least surprise. I think that results from the fact that combinations do not really compose, each option I imagine ws incrementally added to do its own thing and over time we get weird combinations meaning things that are not used combined that frequently but if they are when both are provided what the result would be is not obvious. Obvious I don't think changing the API is a reasonable option at this point, so I think a doc warning of "here be dragons" is good enough and we can all move on.

Thanks

Discussed during sig-etcd triage meeting, it looks like we don't have consensus to proceed with the code change and a docs change would be a safer way forward. Closing in favor of a docs update we can progress under the open issue.